Dinakar feature nutrihelp #161
Conversation
There was a problem hiding this comment.
Hi Dinakar,
Thanks for the detailed description and the security-focused improvements. Hashing refresh tokens and improving session invalidation are the right direction and align well with OWASP guidance.
Requesting changes before merge due to a few high-impact issues that need tightening, given that this touches core auth/session logic:
-
services/supabaseClient.js: environment values (Supabase URL / keys) are currently logged. These must be removed to avoid leaking secrets into logs or CI output.
-
authService refresh/logout flow: the hashed refresh-token approach is good, but the current implementation fetches all active sessions and performs bcrypt comparisons in a loop. This can become expensive at scale and may introduce DoS risk. Please narrow the lookup (e.g. via token identifier / hash prefix / user scope) and ensure logout always updates by session id, not raw refreshToken.
-
middleware/authenticateToken.js: the diff shows duplicate declarations / possible merge artifacts and mixed error paths. Please clean this up and keep consistent 401 responses for missing, expired, and invalid tokens.
-
test-authflow.js / testcapstone.js: these appear to be local/manual validation scripts and currently log tokens and credentials. Suggest moving them under a dev-only scripts folder and avoiding logging raw tokens.
Once these are addressed, I’m happy to re-review. The overall security direction is solid, but we need this implementation to be safe, clean, and production-ready before merging into master.
Thanks!
King Hei
|
Hey King, |
|
Hi Dinakar, Thanks for the update, it is good progress overall. I can’t approve this PR yet because Git is reporting merge conflicts in These conflicts are caused by overlapping changes between this branch and master, where both sides modified the same authentication logic (token parsing, validation, and error handling). Git isn’t able to auto-resolve which version to keep. Suggested fix: Pull / rebase the latest master into this branch Manually resolve the conflicts by keeping one clean, consolidated implementation of: Authorization header parsing Missing token handling Access-token type validation Remove all conflict markers (<<<<<<<, =======, >>>>>>>) Verify login and one protected endpoint still work Once the conflicts are resolved and pushed, I’m happy to re-review and approve 👍 |
Dinakar77
force-pushed
the
dinakar_feature_nutrihelp
branch
from
9b2d89c to
3c5c31a
Compare
|
Hey King Hei, Resolved Row Level Security (RLS) issues by separating anon access from service-role access for session inserts and updates. Improved session lifecycle management by correctly invalidating old sessions on login, refresh, logout, and logout-all. Cleaned up the access-token middleware by removing duplicate logic and standardising error handling for invalid or expired tokens. Removed debug scripts and sensitive logs (raw tokens, secrets) to align with security and team workflow guidelines. |
|
Hi Dinakar, Thanks for the detailed update and the context 👍 Since this PR introduces significant changes to core authentication logic and Supabase access patterns, I’m taking a safety-first approach to make sure there’s no risk of breaking the overall system, especially given this is a multi-trimester project. Before I approve, could you please help confirm two more things:
Once that confirmation is in place and all checks remain green, I’m happy to approve and move this forward. Thanks again for the solid work on this. King Hei |
|
Hey King, This is the screenshot of the service role key that I use |
elanlaw1206
left a comment
elanlaw1206
left a comment
There was a problem hiding this comment.
Thanks for the confirmation and auth flow evidence.
All checks look good, approving this PR.
2 participants
Implemented a secure refresh token flow using hashed refresh tokens with a searchable lookup hash to prevent token leakage and improve performance.
Fixed the refresh token validation logic, ensuring correct session lookup, expiry checks, user status verification, and proper token rotation.
Updated logout and logout-all functionality to invalidate sessions safely without relying on raw refresh tokens.
Cleaned up authentication middleware to remove duplicate logic and ensure consistent error handling.
Removed local debug and test scripts (test-authflow.js, testcapstone.js) to avoid leaking credentials and keep the repository production-ready.
Removed sensitive logging related to Supabase configuration and secrets.